Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove deprecated classes from app-model #41939

Merged
merged 1 commit into from
Jul 17, 2024
Merged

Remove deprecated classes from app-model #41939

merged 1 commit into from
Jul 17, 2024

Conversation

gastaldi
Copy link
Contributor

I am not sure if it's too early to remove them but at least it would be good to have it gone in the next LTS

@gastaldi gastaldi requested a review from aloubyansky July 16, 2024 20:42
@quarkus-bot quarkus-bot bot added area/core area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/gradle Gradle labels Jul 16, 2024
@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Jul 17, 2024 •
edited by github-actions bot
Loading

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 700f48d.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/smallrye-reactive-messaging/deployment

✖ io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector - History

  • Expecting actual: ["-4","-5","-6","-7","-8","-9","-10","-11"] to start with: ["-3", "-4", "-5", "-6"] - java.lang.AssertionError
java.lang.AssertionError: 

Expecting actual:
  ["-4","-5","-6","-7","-8","-9","-10","-11"]
to start with:
  ["-3", "-4", "-5", "-6"]

	at io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector(ConnectorChangeTest.java:36)

@aloubyansky aloubyansky merged commit 731c915 into quarkusio:main Jul 17, 2024
52 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.13 - main milestone Jul 17, 2024
@gtroitsk gtroitsk mentioned this pull request Jul 18, 2024
Merged
11 tasks
@rsvoboda
Copy link
Member

@gastaldi @aloubyansky just checking about RHBQ implications as the deprecation was done in 3.11 (thus not in the latest RHBQ 3.8) through #40224.

Are these classes in scope of any support level from RHBQ side?

aloubyansky
aloubyansky reviewed Jul 22, 2024
View reviewed changes
Copy link
Member

@aloubyansky aloubyansky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The methods that exposed the removed API seem to be deprecated a long time ago (2.4.0.CR1), so those could be removed.
We could still keep the AppArtifact* classes around but they wouldn't be used in the quarkus repository at least. Theoretically, there could be uses of those in other extension repositories, which would be easy to fix.
If we want to be strict about this, we could add the classes back, we could also wait until somebody complains before doing that.

core/deployment/src/main/java/io/quarkus/deployment/ApplicationArchive.java
* @deprecated in favor of {@link #getKey()}
* @return the artifact key or null if not available
*/
AppArtifactKey getArtifactKey();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method was deprecated in 2.4.0.CR1

core/deployment/src/main/java/io/quarkus/deployment/ApplicationArchiveImpl.java
* @deprecated in favor of {@link #getKey()}
* @return archive key
*/
public AppArtifactKey getArtifactKey() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method was deprecated in 2.4.0.CR1

@gsmet
Copy link
Member

gsmet commented Jul 22, 2024

Sorry to be that guy but we need the deprecation to land in an LTS before a removal.
So I would reintroduce the classes.
Even if we don't have a public extension using them, we could have a private one.

@gastaldi gastaldi deleted the appmodel branch July 22, 2024 11:11
@gastaldi
Copy link
Contributor Author

These methods were deprecated since 2.x, we had 2 LTS with them being deprecated already, so I don't understand why we need to reintroduce them?

@rsvoboda
Copy link
Member

My comment was about App* classes deprecated in 3.11 through #40224

@gastaldi gastaldi restored the appmodel branch July 22, 2024 11:17
@gastaldi gastaldi deleted the appmodel branch July 22, 2024 11:18
@gastaldi
Copy link
Contributor Author

How about if we introduced an OpenRewrite script to migrate its usage? Anyway, not opposed to it, just considering options

@gsmet
Copy link
Member

gsmet commented Jul 22, 2024

We can introduce an OpenRewrite recipe, that's indeed a good idea if feasible.

But that doesn't change the rule :).

@aloubyansky
Copy link
Member

That would be mostly for extension code though, not applications.

@gsmet
Copy link
Member

gsmet commented Jul 22, 2024

The recipes also handle updating extensions.

@gsmet
Copy link
Member

gsmet commented Jul 22, 2024

@gastaldi btw, I need the classes back for tomorrow. I don't want to release 3.13.0 without them. Thanks!

@gastaldi gastaldi restored the appmodel branch July 22, 2024 14:22
@gastaldi gastaldi deleted the appmodel branch July 22, 2024 15:57
@gastaldi gastaldi mentioned this pull request Jul 22, 2024
Merged
@gastaldi
Copy link
Contributor Author

@gsmet PR created in #42052

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aloubyansky aloubyansky aloubyansky approved these changes

Assignees
No one assigned
Labels
area/core area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/gradle Gradle triage/flaky-test
Projects
None yet
Milestone
3.13.0.CR1
Development

Successfully merging this pull request may close these issues.
4 participants
@gastaldi @rsvoboda @gsmet @aloubyansky